Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(jmc-agent): clean up probe template utility #241

Merged
merged 3 commits into from
Jun 22, 2023
Merged

chore(jmc-agent): clean up probe template utility #241

merged 3 commits into from
Jun 22, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Jun 22, 2023

Fixes #239

Changes include:

  • addTemplate should check if template file exists before validating.
    • Add more contexts in case file-exist error occurs.
    • ProbeTemplate should correctly set its name.
  • addTemplate should return the ProbeTemplate object so that the caller can directly call serialize on it.
  • Input streams should properly close using the custom trulyClose.
  • Rename methods to match returned values.

@tthvo tthvo added the chore Refactor, rename, cleanup, etc. label Jun 22, 2023
@tthvo tthvo mentioned this pull request Jun 22, 2023
Merged
7 tasks
@tthvo
Copy link
Member Author

tthvo commented Jun 22, 2023

I found that the template file is actually put into storage despite the API returning 400. This causes duplicate upload error when you try to upload the valid template with the same name.

After some digging, looks like validation (i.e. for the template specified in #238) is behaving differently between:

With this PR changes, Cryostat will only call addTemplate when uploading. Hence, in the UI, upload will succeed so that attempts to send duplicate filename will fail with message about file already existing. This closes the issue in #239.

However, reloading the UI page will show the same issue in #238.

maxcao13
maxcao13 approved these changes Jun 22, 2023
View reviewed changes
Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing with this and https://github.com/cryostatio/cryostat/pull/1563 seems to fix the issues!

@tthvo tthvo changed the title chore(jmc-agent): clean up probetemplate utility chore(jmc-agent): clean up probe template utility Jun 22, 2023
@andrewazores andrewazores merged commit 8c1adac into cryostatio:main Jun 22, 2023
@tthvo tthvo deleted the agent-template branch June 22, 2023 21:54
mergify bot pushed a commit that referenced this pull request Jun 22, 2023
@mergify
chore(jmc-agent): clean up probe template utility (#241)
fb639ca 
* chore(template): correct template name for logs

* chore(template): return template when uploading

* fix(template): should truly close the inputstream

(cherry picked from commit 8c1adac)
@mergify mergify bot mentioned this pull request Jun 22, 2023
Merged
andrewazores pushed a commit that referenced this pull request Jun 22, 2023
@mergify
chore(jmc-agent): clean up probe template utility (#241) (#242)
1cb41eb 
* chore(template): correct template name for logs

* chore(template): return template when uploading

* fix(template): should truly close the inputstream

(cherry picked from commit 8c1adac)

Co-authored-by: Thuan Vo <thvo@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxcao13 maxcao13 maxcao13 approved these changes

@andrewazores andrewazores andrewazores approved these changes

Assignees
No one assigned
Labels
backport chore Refactor, rename, cleanup, etc. fix
Projects
No open projects
2.3.1 release
Status: Done
2.4.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Cannot upload JMC Agent probe templates after a failure
3 participants
@tthvo @andrewazores @maxcao13